Set uploaded asset metadata schemaKey to "Asset"#1886
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1886 +/- ##
==========================================
+ Coverage 76.94% 76.96% +0.01%
==========================================
Files 88 88
Lines 12872 12882 +10
==========================================
+ Hits 9905 9914 +9
- Misses 2967 2968 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c47044b to
f8b92a7
Compare
|
@yarikoptic Please take a look at this PR. We will need this merged and released in order to have the integration tests in dandi/dandi-schema#419 to become green. |
yarikoptic
left a comment
There was a problem hiding this comment.
duplication needs to be addressed.
In the longer run I feel that it might need to be the server which would do the upgrade from BaseAsset to Asset!?
| """ | ||
| # Whatever the metadata was gathered as (e.g. a ``BareAsset``), | ||
| # what is being created on the server is an ``Asset`` as result of the upload. | ||
| # The server stores ``schemaKey`` verbatim, so set it |
There was a problem hiding this comment.
I twitch constantly due to my duplication allergy and seeing this length comment + the same code change all around. We have 4 places with that. Even adding a set_asset_schemaKey(metadata) to concentrate that comment + change would be a better solution IMHO.
But I also wonder if we could just add to base class and then extend in derived smth like def prepare_metadata_for_server(metadata: dict) -> metadata where then Asset class would do its assignment there. And then we would call the method when providing metadata.
This would also allow for downgrades of the version as we are yet to do for properly allowing newer client + schema operation with older deployment where feasible.
There was a problem hiding this comment.
The actual code duplication here was a single line (metadata["schemaKey"] = "Asset"); what really repeated was the explanatory comment. I've bound that comment and the operation together in one set_asset_schema_key() helper in dandi.dandiapi, called from all four upload sites, so the documentation and the assignment now live in one place.
On the prepare_metadata_for_server method form specifically: the four sites don't share a base class to hang it on. Two are LocalAsset.iter_upload overrides (dandi/files/) and two are RemoteAsset.set_raw_metadata overrides (dandi/dandiapi.py), so an overridable method would have to be duplicated onto two unrelated bases (or added via a mixin), whereas a plain module-level helper is callable from all four. The version-downgrade motivation is real, but I'd argue that belongs in dandischema (schema migration) rather than the CLI, and it isn't needed for this PR.
The right long-term home for this is the server: a client should upload an unmodified BareAsset, and dandi-archive should validate it and transform it into an Asset (the dandi/dandi-schema#205 spirit). I kept the change client-side deliberately, since the proper server version needs endpoint hardening, likely a database migration, and cross-team discussion, so it is out of scope for this PR. The set_asset_schema_key() helper already gives a single point of truth on the client, and there is a design reason not to push a quick fix into dandi-archive instead (details below). Shall I file a follow-up issue?
Why not patch dandi-archive now: the smell, the migration, and endpoint hardening
Why not a quick server-side patch. There is a shortcut that avoids any migration: setting schemaKey="Asset" inside strip_metadata stamps every stored asset without touching ASSET_COMPUTED_FIELDS or the constraint (strip_metadata runs on both asset write paths, and schemaKey stays a non-computed field, so a draft still has no computed keys). But it makes the server silently rewrite client-supplied metadata. dandi-archive already discards client-supplied computed fields in strip_metadata, yet that is at least explicit; hiding a schemaKey rewrite in the same place amplifies the "the client sends data the server quietly treats as wrong" smell, which is itself the argument for the formal validate-and-transform endpoint. So I'd rather not build further on the current design.
Done cleanly, it entails a migration. The natural way for the server to own schemaKey is to treat it as a computed field, i.e. add it to ASSET_COMPUTED_FIELDS so strip_metadata drops it from the stored (draft) metadata and full_metadata sets it to "Asset" on read. But ASSET_COMPUTED_FIELDS is baked into the asset_metadata_no_computed_keys_or_published CheckConstraint (a draft's metadata must contain none of those keys), so changing the list needs a RemoveConstraint/AddConstraint migration; and since every existing asset stores schemaKey today (it isn't stripped), every existing draft row would then violate the amended constraint, so a data migration must first strip schemaKey from all draft rows. Migration 0011_asset_access_metadata.py is the precedent: it rewrote every asset's metadata and re-created this same constraint for the access field.
The received BareAsset should be validated at the endpoint, and that is a separate check from validating the outcome. A BareAsset arriving at the endpoint should be validated server-side, synchronously at the point of receipt, against the Pydantic BareAsset model (or its JSON schema). That is distinct from validating the transformed outcome, the Asset (and, at publish, PublishedAsset). Today neither of those is true: ingestion is permissive and does no BareAsset validation, AssetRequestSerializer.metadata is a bare serializers.JSONField() whose validate() only checks blob/zarr exclusivity, that path is present and well-formed, and defaults schemaVersion; _create_asset stores the metadata after full_clean(validate_constraints=False); and the only dandischema check runs after storage, in an async task → validate_asset_metadata, which validates the outcome against PublishedAsset and merely records a VALID/INVALID status without rejecting the upload. So a proper design would add synchronous BareAsset validation at receipt, kept separate from that outcome validation.
Whatever the metadata was gathered as (e.g. a BareAsset), what is created on the server is an Asset. The server stores schemaKey verbatim and does not normalize it, so set it at the point of upload rather than relying on the caller. This matters because dandi-schema is changing BareAsset.schemaKey from "Asset" to "BareAsset". Metadata gathered via a BareAsset then serializes with schemaKey="BareAsset", so an asset created or updated from it would be stored with the wrong schemaKey. Pin it at every asset-endpoint write: iter_upload (LocalFileAsset, ZarrAsset) and set_raw_metadata (RemoteBlobAsset, RemoteZarrAsset); the latter is reached by the reextract command without going through iter_upload. A no-op against the current dandi-schema release (where BareAsset.schemaKey is still "Asset"), so it can ship first. Co-Authored-By: Claude Code 2.1.202 / Claude Opus 4.8 claude-opus-4-8 <noreply@anthropic.com>
`test_nwb2asset` and `test_nwb2asset_remote_asset` compared the `nwb2asset()` result against a `BareAsset` built with an explicit `schemaKey="Asset"`, which just restates the field default. `model_construct()` fills defaults in, so dropping it is a no-op against the current dandischema and ensures the equalities continue to hold once dandi-schema#419 lands, where `BareAsset.schemaKey` becomes "BareAsset".
… value `prepare_metadata()` returns a `BareAsset`, whose top-level `schemaKey` is `"Asset"` with the current dandischema but `"BareAsset"` under dandi/dandi-schema#419 (which pins `BareAsset.schemaKey` to its class name). To keep this test passing against either dandischema version, the expected `schemaKey` is set to the default of the `schemaKey` field in `BareAsset` before the comparison, and the four `metadata2asset*.json` files now store a placeholder for the field. The later `validate()` call checks the data against the `Asset` class, so `data_as_dict` is promoted to an Asset (`schemaKey = "Asset"`) beforehand.
The four sites that pin `schemaKey` to `"Asset"` for the server (the two `iter_upload` methods and the two `RemoteAsset.set_raw_metadata` methods) each repeated the assignment and an identical explanatory comment. Move both into a single `set_asset_schema_key()` helper in `dandi.dandiapi` and call it from each site, so the operation and its rationale live in one place.
Spell out, in the placeholder value itself, that `test_prepare_metadata` adjusts the top-level `schemaKey` to the default of the `schemaKey` field in the Pydantic `BareAsset` model, so a reader of the file understands the value is a stand-in rather than the compared value. Co-Authored-By: Yaroslav Halchenko <39889+yarikoptic@users.noreply.github.com> Co-Authored-By: Claude Code 2.1.202 / Claude Opus 4.8 claude-opus-4-8 <noreply@anthropic.com>
afe554f to
f98a949
Compare
Summary
Pin
schemaKeyto"Asset"on every metadata dict written to an asset endpoint. This is the lockstep client change for dandi/dandi-schema#419, which changesBareAsset.schemaKeyfrom"Asset"to"BareAsset".Whatever the metadata was gathered as (e.g. a
BareAsset), what is created on the server is anAsset. The archive storesschemaKeyverbatim and does not normalize it, so onceBareAsset.schemaKeybecomes"BareAsset",BareAsset-derived metadata would be stored with the wrongschemaKey. Setting it at the point of upload keeps this correct regardless of how the metadata was gathered.It is a no-op against the current dandi-schema release (where
BareAsset.schemaKeyis still"Asset"), so it can merge and release ahead of the dandi-schema change.What changed
schemaKeyis set to"Asset"at every sink that writes to an asset create/update endpoint:LocalFileAsset.iter_uploadandZarrAsset.iter_upload(POST create / PUT replace), next to the existingmetadata.setdefault("path", ...)step. Covers the normal CLI upload, the deprecatedupload_raw_asset/iter_upload_raw_asset, and directLocalAsset.upload()/iter_upload()API use.RemoteBlobAsset.set_raw_metadataandRemoteZarrAsset.set_raw_metadata(PUT update). Thereextractservice command reaches these withBareAsset-derived metadata (nwb2assetreturns aBareAsset) without going throughiter_upload.RemoteDandisetVersion.set_raw_metadatais left untouched: it writes Dandiset metadata, not an asset.Test adaptations for the
BareAsset.schemaKeychangeTwo metadata tests hard-coded the top-level
schemaKeyof aBareAssetas"Asset", which becomes"BareAsset"under dandi/dandi-schema#419. Both now track the default of theschemaKeyfield inBareAssetrather than a literal, so they pass against the current dandischema and #419 alike:test_nwb2asset/test_nwb2asset_remote_asset: drop the redundant explicitschemaKey="Asset", which merely restated the field default.test_prepare_metadata: set the expectedschemaKeyto the default of theschemaKeyfield inBareAssetbefore the comparison (the fourmetadata2asset*.jsonfiles now store a placeholder for it), and promote the object to anAssetbefore thevalidate()call that checks it against theAssetclass.Test plan
black --checkclean on the changed files (verified out-of-band; the repo's pinned black currently crashes under Python 3.14, unrelated to this change)test_upload_downloadextended to assert the uploaded asset's storedschemaKey == "Asset"(needs the Docker-backed archive fixtures in CI)BareAsset.schemaKey == "BareAsset") via DO NOT MERGE: check #1886 schemaKey handling against dandi-schema #419 #1888, which repoints thedandischemadependency at the Consolidate Asset/Dandiset models; gate publish validation on datePublished dandi-schema#419 branch🤖 Generated with Claude Code